-
Notifications
You must be signed in to change notification settings - Fork 182
[CIR] Emit bitcast for equal-width types #1991
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Implement VisitAsTypeExpr to lower AsTypeExpr expressions. - Emit an error when source and destination types differ in bitwidth. - When types already match, return the source value (no-op). - Otherwise lower to a CIR bitcast using cir::CastOp with CastKind::bitcast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I normally don't do code reviews, but you should follow the skeleton for this function from OG codegen — see. We try not to deviate too much from it unless there's a strong enough reason for that.
| @@ -0,0 +1,9 @@ | |||
| // RUN: %clang -target x86_64-unknown-linux-gnu -cl-std=CL3.0 -Xclang -finclude-default-header -Xclang -fclangir -emit-cir %s -o - | FileCheck %s | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments regarding testing (Do we have documentation on this? @bcardosolopes)
Your tests should ideally check for the three things:
- CIR block: Tests your lowering to MLIR CIR dialect (
-fclangir -emit-cir) - LLVM block: Tests lowering to LLVM IR through the CIR pipeline (
-fclangir -emit-llvm) - OGCG block: Tests lowering through the OG pipeline (no
-fclangir). This confirms parity with OG.
For reference, see the CUDA tests: https://github.com/llvm/clangir/blob/main/clang/test/CIR/CodeGen/CUDA/builtins-nvptx-ptx60.cu
For AsTypeExpr, the OG test suite is here: https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGenOpenCL/as_type.cl
I would suggest you try to replicate what is supported as much as possible for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have text for it in clangir.org unfortunately, if you'd like to contrib some all is needed is a PR against branch gh-pages? There's a placeholder here: https://llvm.github.io/clangir/GettingStarted/coding-guideline.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have text for it in clangir.org unfortunately, if you'd like to contrib some all is needed is a PR against branch gh-pages? There's a placeholder here: https://llvm.github.io/clangir/GettingStarted/coding-guideline.html
Sweet — I'll definitely do that when I have time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I updated the implementation to follow the original codegen
structure. I kept vec3 <-> vec4 cases unimplemented because our project never
hits those paths, and I prefer not to add tests for them since clang exits
on the error and all tests would need to be split.
Only the same-size bitcast path is implemented, which is the only case CIR
currently needs.
Please do whenever you feel like, we really appreciate it :) |
| llvm_unreachable("NYI"); | ||
| } | ||
|
|
||
| // If types are identical, return the source |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these last two cases, I'd suggest implementing the createCastsForTypeOfSimeSize function as it is similarly done in OG (see:
clangir/clang/lib/CodeGen/CGExprScalar.cpp
Line 5798 in 96a0551
| static Value *createCastsForTypeOfSameSize(CGBuilderTy &Builder, |
static mlir::Value createCastsForTypeOfSameSize(CIRGenBuilderTy &builder,
const cir::CIRDataLayout &dl,
mlir::Value src, mlir::Type dstTy,
) {
auto srcTy = Src.getType();
// Case 1.
if (mlir::isa<cir::PointerType>(srcTy) && mlir::isa<cir::PointerType>(dstTy))
return Builder.createBitcast(src, dstTy);
// Other casesFeel free to assert if anything is not implemented yet or if it's out of the scope of this PR.
This patch adds support for emitting a
cir.bitcastwhen reinterpretingvalues that have the same bit-width. This enables correct handling of
vector reinterpretation in CIR and aligns with the behavior of the LLVM IR
lowering.
Previously, equal-width reinterpretations were not handled, which caused
assertions or incorrect lowering when working with vector types or other
equal-sized aggregates.
Key points:
cir.bitcastemission when source and destination types haveequal width and only reinterpretation is required.
current type conversion pipeline.
Testing: